Skip to content
This repository was archived by the owner on Jan 30, 2020. It is now read-only.

Conversation

@neilime
Copy link
Contributor

@neilime neilime commented Oct 1, 2015

Prevents non well formatted accept header value.

Fix #21

Test params string before exploding it
@Freeaqingme
Copy link
Member

Awesome!

As you can see below one of the CS checks failed, apparently on some trailing spaces. Something you could perhaps take a look at as well?

Results are here: https://travis-ci.org/zendframework/zend-http/jobs/83107792

@neilime
Copy link
Contributor Author

neilime commented Oct 12, 2015

Done !

@Freeaqingme
Copy link
Member

@Maks3w Is this something you could look at? I've been out of the game for quite some time (although I still feel for this particular component), so I am a little uncomfortable merging it. As far as I'm concerned it's ready to go!

@Maks3w
Copy link
Member

Maks3w commented Oct 17, 2015

Well I have questions.

  1. Because an abstract class has been modified then any child test should have a test and not only Abstract-Language
  2. After take a look to http://www.w3.org/Protocols/rfc2616/rfc2616-sec14.html I don't see those headers should share the same ABNF so I think the abstract class should be split with trait functions and be deprecated

@Maks3w
Copy link
Member

Maks3w commented Oct 17, 2015

Errata: RFC2616 are expressed using EBNF

Accept-Header is defined individually on RFC3282

http://tools.ietf.org/html/rfc7231#section-5.3 contains definitions around the whole AcceptX headers

@Maks3w
Copy link
Member

Maks3w commented Oct 17, 2015

After reading RFC 7231 only the weight param (;q=[0,0.001-1]) is common to all those headers but the rest of parsing rules are not.

This is the ABNF resume from http://tools.ietf.org/html/rfc7231#appendix-D

Accept = [ ( "," / ( media-range [ accept-params ] ) ) *( OWS "," [OWS ( media-range [ accept-params ] ) ] ) ]
Accept-Charset = *( "," OWS ) ( ( charset / "*" ) [ weight ] ) *( OWS "," [ OWS ( ( charset / "*" ) [ weight ] ) ] )
Accept-Encoding = [ ( "," / ( codings [ weight ] ) ) *( OWS "," [ OWS ( codings [ weight ] ) ] ) ]
Accept-Language = *( "," OWS ) ( language-range [ weight ] ) *( OWS "," [ OWS ( language-range [ weight ] ) ] )

accept-params = weight *accept-ext
weight = OWS ";" OWS "q=" qvalue

OWS = Optional White Space

@Freeaqingme
Copy link
Member

@Maks3w How is this all relevant for this particular pull request? Fwiw, the implementations that are included in zend\http for the various accept header types come with own regexes to support and validate the different formats.

Of course we can always refactor this class and deprecate parts of it. But that seems way beyond the scope of this particular issue: Ensuring no notices are thrown on invalid input.

@Maks3w
Copy link
Member

Maks3w commented Oct 17, 2015

Reading the RFC and the ABNF resume split languages with ; is not allowed. Should be ,. So the proposed solution is wrong. The header was malformed and there is nothing to fix.

@Maks3w
Copy link
Member

Maks3w commented Nov 1, 2015

Closed due inactivity.

@Maks3w Maks3w closed this Nov 1, 2015
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants